Skip to content

Conversation

@xokdvium
Copy link
Contributor

Motivation

It's not being excersised by anything and already contains bugs (like the fact that copyRecursive only handles the first entry in a directory). Evidently, it can just be removed and was never used by anything.

Context

#14597 (comment)


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

It's not being excersised by anything and already contains
bugs (like the fact that copyRecursive only handles the first entry
in a directory). Evidently, it can just be removed and was never used
by anything.
@xokdvium xokdvium requested a review from edolstra as a code owner November 20, 2025 19:23
@xokdvium
Copy link
Contributor Author

There seems to have been some back-and-forth with this code #10084. Pretty sure it's actually dead this time? @Ericson2314

@roberth roberth requested a review from Ericson2314 November 20, 2025 20:16
@Ericson2314
Copy link
Member

I need to keep spelunking, but

are some of my unmerged PRs for more git tree hashing stuff

Comment on lines -38 to -40
for (auto & [name, _] : accessor.readDirectory(from)) {
copyRecursive(accessor, from / name, dirSink, relDirPath / name);
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the buggy line in question. break shouldn't be there

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I end up needing this, we can revert.

@Ericson2314 Ericson2314 added this pull request to the merge queue Nov 20, 2025
@xokdvium
Copy link
Contributor Author

Seems like there's a good use-case for copyRecursive. So I'll fix it instead then...

@xokdvium xokdvium removed this pull request from the merge queue due to a manual request Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants